Skip to content

support loading COG layers from query params #9531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dsuren1 merged 6 commits into from
Apr 10, 2024
Merged

support loading COG layers from query params #9531

dsuren1 merged 6 commits into from
Apr 10, 2024

Conversation

@landryb
Copy link
Contributor

@landryb landryb commented Oct 2, 2023 •
edited
Loading

Description

implements #9527

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Feature

Issue

What is the current behavior?

fixes #9527

What is the new behavior?
http://localhost:8081/#/viewer/new?actions=%5B%7B%22type%22:%22CATALOG:ADD_LAYERS_FROM_CATALOGS%22,%22layers%22:%5B%22testtitle%22%5D,%22sources%22:%5B%7B%22type%22:%22cog%22,%22url%22:%22https%3A%2F%2Fcogeo.craig.fr%2Fopendata%2Fortho%2Forthocraig3_vichy_2021.cog.tif%22%7D%5D%7D%5D should work and zoom to the provided COG

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@landryb
Copy link
Contributor Author

landryb commented Oct 2, 2023

@dsuren1 help/feedback welcome on this. Even with projectionDefs containing EPSG:2154 i cant get the layer to be automagically added.

@landryb
Copy link
Contributor Author

landryb commented Oct 2, 2023

ok it loads the cog with your tweak from #9527 (comment) but im not comfy with it.. maybe i should set name to the value of text to avoid having a specialcase like this or that can lead to conflicts later on ?

also, it doesn't automatically zoom to the COG extent even if it knows its bbox (i have the 'zoom to layer' button in the toc), while it's the case when the COG is added from the catalog.

@landryb
Copy link
Contributor Author

landryb commented Oct 2, 2023 •
edited
Loading

with 9f2f3c9 it works but i have to figure out what triggers the zoom to layer behaviour

edit bah, dunno where i got that from, but loading a WMS layer from query params doesn't automagically zoom to the WMS layer either, so... a non-existent feature i might have dreamt of. setting PR to reviewable, feedback welcome

@landryb landryb marked this pull request as ready for review October 2, 2023 15:02
This was referenced Oct 3, 2023
Closed
Merged
@landryb landryb mentioned this pull request Jan 15, 2024
Closed
@landryb
Copy link
Contributor Author

landryb commented Jan 15, 2024

branch rebased on top of #9590 & #9508, will do more tests locally backported on 2023.02, but feedback, testing & review welcome.

@tdipisa
Copy link
Member

tdipisa commented Jan 15, 2024 •
edited
Loading

Thank you so much for contributing @landryb. We will check this asap. This is still WIP n your side as reported din description?

@tdipisa tdipisa requested a review from dsuren1 January 15, 2024 14:53
@tdipisa tdipisa added this to the 2024.01.00 milestone Jan 15, 2024
@landryb
Copy link
Contributor Author

landryb commented Jan 15, 2024 •
edited
Loading

Thank you so much for contributing @landryb. We will check this asap. This is still WIP n your side as reported din description?

no this is ready for review, eslint is happy with the js changes, and i've just tested it backported on a local 2023.02 build of ms2-georchestra and it does what i expect, eg ?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["cog clerco cc46"],"sources":[{"type":"cog","url":"https%3A%2F%2Fcogeo.craig.fr%2Fopendata%2Fortho%2Fortho25_cc46_clerco_1996.cog.tif"}]}] properly loads the COG at initial ms2 loading.

image

@landryb
Copy link
Contributor Author

landryb commented Jan 15, 2024

updated the issue description to state that it wasnt just WIP, but working for me :) it was already working in the initial PR from october, but it had stalled and bitrotted.

@landryb
Copy link
Contributor Author

landryb commented Feb 23, 2024

can i get a review on this before it bitrots more ? should i rebase the branch or a merge commit like f22d540 above is acceptable ?

@dsuren1
Copy link
Contributor

dsuren1 commented Feb 23, 2024

@landryb we are waiting for this PR to be merged as it refactors the COG workflow along with options to style. I think you may have to rebase again to include the new changes after the linked PR is merged.

@tdipisa tdipisa modified the milestones: 2024.01.00, 2024.01.01 Mar 4, 2024
@dsuren1
Copy link
Contributor

dsuren1 commented Mar 22, 2024

@landryb Could you please align your changes with latest master. Thanks!

@landryb
Copy link
Contributor Author

landryb commented Mar 25, 2024

@landryb Could you please align your changes with latest master. Thanks!

sure, will try to understand what happened in #9857 & #10089 wrt my changes.. from my understanding i should reuse LayerUtils.getLayerConfig instead of my own fetchMetadata (added in 461da07, used in 71899b3) .. and test.

@landryb
Copy link
Contributor Author

landryb commented Mar 25, 2024

@dsuren isnt there a problem with LayerUtils.getLayerConfig added in d683b82 ? the order of parameters doesn't seem to be match between the call in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/api/catalog/COG.js#L57:

return LayerUtils.getLayerConfig({url, controller, layer})

and the API in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/utils/cog/LayerUtils.js#L56

export const getLayerConfig = ({ url, layer, controller })

i dunno if that matters.. but whatever order of parameters i put in my code, eslint complains if i try building the layer object/parameter on the fly:

Line 72, column 52: /data/src/georchestra/mapstore2-georchestra/MapStore2/web/client/api/catalog/COG.js: Unexpected token (72:52)

  70 |     return Promise.all([...layers]).then((_layers) => {
  71 |         if (!_layers.length) {
> 72 |             return LayerUtils.getLayerConfig({_url, {

eslint is happy if i build the layer object before, as done in 0260ac7 (untested yet)

@dsuren1
Copy link
Contributor

dsuren1 commented Mar 25, 2024 •
edited
Loading

@landryb

getLayerConfig takes object as a parameter, the order of the prop is not a concern but the name of the prop passed. Modify your code as shown below

return LayerUtils.getLayerConfig({url: _url, layer, controller})

@landryb
Copy link
Contributor Author

landryb commented Mar 25, 2024

@landryb

getLayerConfig takes object as a parameter, the order of the prop is not a concern but the name of the prop passed. Modify your code as shown below

return LayerUtils.getLayerConfig({url: _url, layer, controller})

ah, thanks, now i understand the logic, it wasnt obvious for me that the variable names were auto matched on object properties. fixed with 60e7269 and i've been able to successfully test it on a locally running instance with ?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["Vichy 3cm"],"sources":[{"type":"cog","url":"https%3A%2F%2Fcogeo.craig.fr%2Fopendata%2Fortho%2Forthocraig3_vichy_2021.cog.tif"}]}] as params

dsuren1
dsuren1 requested changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@landryb
Kindly update the docmap-query-parameters.md for adding COG layer via query paramater similar to other layer type. Also a note in the docs regarding the 'limitation' mentioned below. Thanks!

@offtherailz @tdipisa
The only limitation to adding COG layer via query parameter, is that it is not possible to cancel the metadata fetch call as there is not control exposed at the moment. So when a big source is requested the map load time maybe significantly affected.

dsuren1
dsuren1 requested changes Apr 10, 2024
View reviewed changes
@dsuren1 dsuren1 enabled auto-merge (squash) April 10, 2024 11:44
@dsuren1 dsuren1 merged commit 6b4d662 into geosolutions-it:master Apr 10, 2024
@dsuren1 dsuren1 added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Apr 10, 2024
@dsuren1
Copy link
Contributor

dsuren1 commented Apr 10, 2024

@ElenaGallo Kindly test it in DEV and let us know if it can be backported. Thanks!

@ElenaGallo
Copy link
Contributor

ElenaGallo commented Apr 10, 2024

test passed, @dsuren1 please backport to 2024.01.xx. Thanks

dsuren1 added a commit to dsuren1/MapStore2 that referenced this pull request Apr 11, 2024
@landryb @dsuren1
support loading COG layers from query params (geosolutions-it#9531)
df5d118 
* COG services only have a single layer, use provided layer text as title (geosolutions-it#9527)

* create COG layer on the fly if none is provided (geosolutions-it#9527)

* use properly named parameters for getLayerConfig call

* return layer in case getLayerConfig failed

* add documentation about loading COG layer via viewer parameters (geosolutions-it#9531)

* Update docs/developer-guide/map-query-parameters.md

---------

Co-authored-by: Suren <dsuren1@gmail.com>
(cherry picked from commit 6b4d662)
@dsuren1 dsuren1 mentioned this pull request Apr 11, 2024
Merged
@tdipisa
Copy link
Member

tdipisa commented Apr 16, 2024

@ElenaGallo @dsuren1 this is for 2024.01.01 so let's wait the delivery of 2024.01.00 before merging the backport.

@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label May 14, 2024
tdipisa pushed a commit that referenced this pull request May 14, 2024
@dsuren1 @landryb
support loading COG layers from query params (#9531) (#10187)
c04b96a 
* COG services only have a single layer, use provided layer text as title (#9527)

* create COG layer on the fly if none is provided (#9527)

* use properly named parameters for getLayerConfig call

* return layer in case getLayerConfig failed

* add documentation about loading COG layer via viewer parameters (#9531)

* Update docs/developer-guide/map-query-parameters.md

---------

Co-authored-by: Suren <dsuren1@gmail.com>
(cherry picked from commit 6b4d662)

Co-authored-by: Landry Breuil <landryb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsuren1 dsuren1 dsuren1 approved these changes

Assignees

@landryb landryb

Labels

enhancement External Contribution

Projects

None yet

Milestone

2024.01.01

Development

Successfully merging this pull request may close these issues.

support loading COG layers from query params

4 participants

@landryb @tdipisa @dsuren1 @ElenaGallo